Skip to content

Conversation

@barakmich
Copy link
Contributor

Adds a pattern and a few basic static optimizers to pkg/query to rewrite query plans.

Fixes a deduplication bug when we elide Unions (we might be able to further remove deduplication from unions, tbd)

Also tests both optimized and unoptimized trees for correctness in integration tests.

@barakmich barakmich requested a review from a team as a code owner October 29, 2025 22:25
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 29, 2025
@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 85.98726% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.48%. Comparing base (c090132) to head (5e19282).

Files with missing lines Patch % Lines
pkg/query/optimize.go 77.15% 4 Missing and 4 partials ⚠️
pkg/query/union.go 66.67% 5 Missing and 2 partials ⚠️
pkg/query/optimize_caveat.go 88.89% 2 Missing and 2 partials ⚠️
pkg/query/path.go 85.72% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2669      +/-   ##
==========================================
+ Coverage   79.48%   79.48%   +0.01%     
==========================================
  Files         456      459       +3     
  Lines       47367    47486     +119     
==========================================
+ Hits        37643    37739      +96     
- Misses       6958     6969      +11     
- Partials     2766     2778      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@barakmich barakmich force-pushed the barakmich/optimizers branch 2 times, most recently from c94ede6 to d8d75fb Compare October 30, 2025 23:06
require.NoError(t, err)
require.True(t, changed1)

// Create the same structure again
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? ApplyOptimizations doesn't mutate the inputs so you can reuse it

Copy link
Contributor

@miparnisari miparnisari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 😄

There is one test that fails both locally and in CI, and another one that fails only locally. Are we using --failfast in CI?

$ go test -race . -count=1
# github.com/authzed/spicedb/pkg/query.test
ld: warning: '/private/var/folders/5p/328y159n1334j9czdhzl2ts40000gn/T/go-link-2977391469/000013.o' has malformed LC_DYSYMTAB, expected 98 undefined symbols to start at index 1626, found 95 undefined symbols starting at index 1626
--- FAIL: TestIteratorTracing (0.00s)
    --- FAIL: TestIteratorTracing/Union_tracing (0.00s)
        tracing_test.go:101: 
                Error Trace:    /Users/miparnisari/Documents/GitHub/spicedb/pkg/query/tracing_test.go:101
                Error:          Should be true
                Test:           TestIteratorTracing/Union_tracing
--- FAIL: TestAliasIterator (0.03s)
    alias_test.go:47: 
                Error Trace:    /Users/miparnisari/Documents/GitHub/spicedb/pkg/query/alias_test.go:47
                Error:          "[{{doc1 document} read user:alice#... <nil> <nil> [] map[]}]" should have 3 item(s), but has 1
                Test:           TestAliasIterator
                Messages:       should have 3 rewritten relations
    --- FAIL: TestAliasIterator/Check_BasicRelationRewriting (0.00s)
panic: test executed panic(nil) or runtime.Goexit

goroutine 214 [running]:
testing.tRunner.func1.2({0x105a5c240, 0x1066a1b40})
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:1872 +0x2b4
testing.tRunner.func1()
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:1875 +0x460
runtime.Goexit()
        /opt/homebrew/opt/go/libexec/src/runtime/panic.go:615 +0x60
testing.(*common).FailNow(0xc000102fc0)
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:1013 +0x70
github.com/stretchr/testify/require.Len({0x105c755e8, 0xc000102fc0}, {0x1059d5200, 0xc000948c48}, 0x3, {0xc000659e68, 0x1, 0x1})
        /Users/miparnisari/go/pkg/mod/github.com/stretchr/testify@v1.11.1/require/require.go:1203 +0xe4
github.com/stretchr/testify/require.(*Assertions).Len(0xc0006cc1d0, {0x1059d5200, 0xc000948c48}, 0x3, {0xc000659e68, 0x1, 0x1})
        /Users/miparnisari/go/pkg/mod/github.com/stretchr/testify@v1.11.1/require/require_forward.go:954 +0xac
github.com/authzed/spicedb/pkg/query.TestAliasIterator.func1(0xc000780e00)
        /Users/miparnisari/Documents/GitHub/spicedb/pkg/query/alias_test.go:47 +0x79c
testing.tRunner(0xc000780e00, 0xc000904348)
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:1934 +0x168
created by testing.(*T).Run in goroutine 9
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:1997 +0x6e4
FAIL    github.com/authzed/spicedb/pkg/query    0.626s
FAIL
[15:03:13] ~/Documents/GitHub/spicedb/pkg/query (barakmich/optimizers) $ 

@barakmich barakmich force-pushed the barakmich/optimizers branch 3 times, most recently from 27a1eda to 55b3d05 Compare November 13, 2025 18:17
@barakmich barakmich enabled auto-merge November 13, 2025 18:20
@barakmich barakmich added this pull request to the merge queue Nov 13, 2025
Any commits made after this event will not be merged.
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 13, 2025
@barakmich barakmich enabled auto-merge November 13, 2025 22:24
@barakmich barakmich force-pushed the barakmich/optimizers branch from 55b3d05 to 5e19282 Compare November 13, 2025 22:24
@barakmich barakmich added this pull request to the merge queue Nov 13, 2025
Any commits made after this event will not be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants